-
Notifications
You must be signed in to change notification settings - Fork 406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: enabled_if with env
variable
#10936
Conversation
5bc5a0c
to
46a08b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to get this fixed!
src/dune_rules/enabled_if.ml
Outdated
@@ -29,6 +30,7 @@ let common_vars ~since = | |||
match var with | |||
| "context_name" -> var, (2, 7) | |||
| "arch_sixtyfour" -> var, (3, 11) | |||
| "env" -> var, (1, 14) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this since version 1.14? Given it didn't work before, shouldn't it be 3.17 or as a bugfix to a feature in 3.15 be (3, 15)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related to env
itself. Like context_name
start from 2.7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surely we need a backport on 3.15 after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this is 1.14 either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. I've got a misunderstanding here, reading the error message. I was thinking it was specific to the variable itself. The 1.14
is coming from the doc, when env
is introduced.
Error: %{env:--} is only available since version <version> of the dune
language. Please update your dune-project file to have (lang dune <version>).
Sorry @Leonidas-from-XIV you're right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I add on 3.17
.
src/dune_rules/enabled_if.ml
Outdated
@@ -53,12 +55,12 @@ let decode_value ~allowed_vars ?(is_error = true) () = | |||
Blang.Ast.decode | |||
~override_decode_bare_literal:None | |||
(String_with_vars.decode_manually (fun env var -> | |||
let name = Dune_lang.Template.Pform.name var in | |||
match Dune_lang.Template.Pform.payload var with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this pattern match at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there's no need, I'll refactor. I just kept it because it was there before.
cde37ce
to
00fc5cc
Compare
Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
00fc5cc
to
fd800a0
Compare
Added |
* fix: enabled_if with `env` variable Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com> Signed-off-by: Sylvain78 <sylvain_kerjean@hotmail.com>
Fixes #10905.